[iOS] Fabric: Add missing source properties of Image#46460
Conversation
facebook-github-bot
added
CLA Signed
cipolleschi
left a comment
cipolleschi
left a comment
There was a problem hiding this comment.
Thanks for fixing this!
|
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
cipolleschi
left a comment
cipolleschi
left a comment
There was a problem hiding this comment.
There have been some feedback internally on the change, to improve this.
Could you please have a look at them and apply them?
| method = [[NSString alloc] initWithBytesNoCopy:(void *)imageSource.method.c_str() | ||
| length:imageSource.method.size() | ||
| encoding:NSUTF8StringEncoding | ||
| freeWhenDone:NO] | ||
| .uppercaseString |
There was a problem hiding this comment.
why can't we just do?
| method = [[NSString alloc] initWithBytesNoCopy:(void *)imageSource.method.c_str() | |
| length:imageSource.method.size() | |
| encoding:NSUTF8StringEncoding | |
| freeWhenDone:NO] | |
| .uppercaseString | |
| method = [[NSString alloc] initWithUTF8String:imageSource.method.c_str()].uppercaseString; |
also, no need for the ?: fallback.
There was a problem hiding this comment.
Emm, I used no copy method because I think it can reduce one copy, if we use initWithUTF8String, it would produces twice copy operations from c_str to nsadata.
| length:imageSource.body.size() | ||
| encoding:NSUTF8StringEncoding | ||
| freeWhenDone:NO]; | ||
| body = [RCTConvert NSData:bodyString]; |
There was a problem hiding this comment.
can't we just initialize it with some NSData initializer?
There was a problem hiding this comment.
Seems no suitable initializer...
There was a problem hiding this comment.
the initializer should be:
[NSData dataWithBytes:imageSource.body.c_str() length:imageSource.body.size()]We use this also in other locations, like here
There was a problem hiding this comment.
@cipolleschi I updated. But they are maybe different when bytes are not a valid UTF-8 bytes?
|
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
cipolleschi
left a comment
cipolleschi
left a comment
There was a problem hiding this comment.
See comment in NSData initializer
|
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@cipolleschi merged this pull request in 052def0. |
|
This pull request was successfully merged by @zhongwuzw in 052def0 When will my fix make it into a release? | How to file a pick request? |
4 participants
Summary:
Fabric: Add missing source properties of Image
Changelog:
[IOS] [FIXED] - Fabric: Add missing source properties of Image
Test Plan:
Missing props should worked.